- 
                Notifications
    You must be signed in to change notification settings 
- Fork 431
@W-19368408: [MSDK] Local dev instant login (iOS) #3944
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
@W-19368408: [MSDK] Local dev instant login (iOS) #3944
Conversation
| if ([arguments containsObject:@"-creds"]) { | ||
| NSString *creds = arguments[[arguments indexOfObject:@"-creds"] + 1]; | ||
|  | ||
| [TestSetupUtils populateAuthCredentialsFromString:creds]; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The crux is retrieving the new parameter and passing it to mostly existing test harness logic. @bbirman, is this a logical place to accomplish this? I tried few other ideas, but ran into issues and circular dependencies on the manager singleton.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is SFUserAccountManager a possibility? If not, this class sounds good to me, though wondering about using load, does it have something to do with the runtime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked through SFUserAccountManager just now. It doesn't seem to have any logical place to add this. I tried a couple, but they also trigger some circular initialization overflows a lot like some of the entry points in the manager I tried. TestSetupUtils is dependent on the SDK and user account manager objects.
load is nice in this instance since it's the class level initializer for SalesforceSDKManager and is outside the other initialization methods.  You're correct that it's part of the Objective-C spec.  I recall there is a way to do something like that in Swift, but it has been a few years since I exercised that.
| @return The configuration data used to configure SFUserAccountManager (useful e.g. for hybrid | ||
| apps which need the data to bootstrap SFHybridViewController). | ||
| */ | ||
| + (SFSDKTestCredentialsData *)populateAuthCredentialsFromString:(NSString *)testCredentialsJsonString; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new method lets us reuse the non-file logic around ingesting the JSON.
| return credsData; | ||
| } | ||
|  | ||
| + (void)synchronousAuthRefresh | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method didn't change, though Github marked it as a diff due to other logic moving.
| authListener.returnStatus); | ||
| } | ||
|  | ||
| + (SFOAuthCredentials *)newClientCredentials { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method did not change. This method didn't change, though Github marked it as a diff due to other logic moving.
| return [TestSetupUtils authCredentialsFromJson:[SFJsonUtils objectFromJSONString:testCredentialsJsonString]]; | ||
| } | ||
|  | ||
| + (void)synchronousAuthRefresh | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method didn't change, though Github marked it as a diff due to other logic moving.
| authListener.returnStatus); | ||
| } | ||
|  | ||
| + (SFOAuthCredentials *)newClientCredentials { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method didn't change, though Github marked it as a diff due to other logic moving.
| 
 Clang Static Analysis Issues
 Generated by 🚫 Danger | 
| Codecov Report❌ Patch coverage is  
 ❌ Your patch status has failed because the patch coverage (57.14%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@            Coverage Diff             @@
##              dev    #3944      +/-   ##
==========================================
- Coverage   63.05%   63.00%   -0.06%     
==========================================
  Files         249      249              
  Lines       22455    22462       +7     
==========================================
- Hits        14160    14152       -8     
- Misses       8295     8310      +15     
 
 🚀 New features to boost your workflow:
 | 
…ogin To SalesforceSDKManager `init`)
| if ([arguments containsObject:@"-creds"]) { | ||
| NSString *creds = arguments[[arguments indexOfObject:@"-creds"] + 1]; | ||
|  | ||
| [TestSetupUtils populateAuthCredentialsFromString:creds initializeSdk:NO]; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bbirman - Now we have Local Dev Instant Login hosted in SalesforceSDKManager.init so we can avoid using load.  Notice the two new boolean parameters on these two statements which allow TestSetupUtils to execute correctly here in initializeSDK while still running exactly the same in existing tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
| if (postUserDidLogIn) { | ||
| NSDictionary *userInfo = @{kSFNotificationUserInfoAccountKey: userAccount, | ||
| kSFNotificationUserInfoAuthTypeKey: authInfo}; | ||
| [[NSNotificationCenter defaultCenter] postNotificationName:kSFNotificationUserDidLogIn | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two lines allow TestSetupUtils to stop the default login from being shown and deliver the test app directly to its authenticated view.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That matches the behavior for Local Dev Instant Login for Android.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is similar notification login in the user account manager, but its only a few lines and not visible to this scope, so I thought to leave that alone and replicate the notification here so we minimize authentication regression risk. All the changes in this pull request are non-production code so far.
| @bbirman - After really researching exactly how  | 
| I ran this using the RestAPIExplorer sample and giving it the new parameter shown in the screenshot above. After a fresh install, it brought my test user directly to the app's authenticated view in all my test runs ⭐️ | 
| Local tests are passing as well ✅ | 
…Utils` From Code Coverage)
f8a40a6
      into
      
  
    forcedotcom:dev
  
    
🎸 Ready For Review 🥁
This is the iOS counterpart to forcedotcom/SalesforceMobileSDK-Android#2785.
This adds a new process argument which may be used for debug app builds only to authenticate a user via test credentials in automated tests. The new code is wrapped in a
#ifdef DEBUGpreprocessor condition so that it will not be present for release builds.This new means of automating authentication for debug testing does not expose production builds to risk and also minimizes access since usernames and passwords are not used. The test credentials JSON provided by Salesforce Mobile SDK's REST Explorer sample app uses refresh tokens which can be remotely expired and cannot be used for privilege elevation beyond the public APIs.
Here's an example of testing the new parameter interactively in Xcode by adding it to the app's scheme:

Here's an example for XCUITest:
This is an Appium example:
And, finally, an example at the shell: